Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow selecting field for email recipient without saving first #42

Closed
wants to merge 1 commit into from

Conversation

kinglozzer
Copy link

When adding a new EmailRecipient against a user form, as the record hasn’t been written yet, userforms relies on the LeftAndMain.currentPage session value to find the form:

https://github.com/silverstripe/silverstripe-userforms/blob/b990c9e004e5b13f2afdefd12817f03fa7d5107c/code/Model/Recipient/EmailRecipient.php#L135-L156

That obviously doesn’t work for userform elements, so this is a workaround for that. Currently you have to type in a random email address, save the recipient, and then you can see the dropdown for fields to pick from.

Before:
Screenshot 2019-06-13 at 12 18 34

After:
Screenshot 2019-06-13 at 12 18 54

@kinglozzer kinglozzer force-pushed the recipients-config branch 3 times, most recently from 937dd44 to 2a2cf60 Compare June 13, 2019 11:28
@kinglozzer
Copy link
Author

Bump. I know the fix looks a bit ugly but it makes a big difference to the UX of adding recipients

@GuySartorelli
Copy link
Collaborator

GuySartorelli commented Feb 12, 2023

Should this PR be targetting 4.0 (CMS 5 compatible) or 3 (CMS 4 compatible)? I don't think there's any breaking changes here, so I would have thought 3?

userforms relies on the LeftAndMain.currentPage session value to find the form ... That obviously doesn’t work for userform elements

Just to make sure I'm understanding this correctly - I'm assuming this doesn't work because the currentPage value is set to the actual page (or other object) which owns the elemental area rather than to the userform block edit form?

It would probably be worth adding a comment to the top of the new method explaining that this is a workaround for that session mismatching - that way if we ever change LeftAndMain.currentPage to respect GridField item edit forms (which IMO would be a more correct fix for this) we can come back and revert this change.

@kinglozzer kinglozzer changed the base branch from master to 3 February 13, 2023 10:25
@kinglozzer
Copy link
Author

Just to make sure I'm understanding this correctly - I'm assuming this doesn't work because the currentPage value is set to the actual page (or other object) which owns the elemental area rather than to the userform block edit form?

Yep exactly

@kinglozzer
Copy link
Author

I’ve rebased this now and targeted 3. I’ve also added some more descriptive comments explaining why this code exists

@GuySartorelli
Copy link
Collaborator

Can you please provide step-by-step instructions for reproducing the original problem from a fresh installation? I can't seem to reproduce it with this module in the 3 branch.

@kinglozzer
Copy link
Author

I think to recreate it you need to set:

SilverStripe\UserForms\Model\Recipient\EmailRecipient:
  allow_unbound_recipient_fields: true

If you then create a form that has at least one field with the Email type, then when you add a new email recipient against a (elemental) userform, you’ll see the “Before” screenshot above which includes the message:

You will be able to select from valid form fields after saving this record.

To select from the list of email fields defined in the form, you have to enter an email and save it first, then you’ll see the dropdown with a list of fields to choose from.

After this change, you’ll see the “After” screenshot where you can immediately select an email address from the list of user defined fields before writing for the first time.

@GuySartorelli
Copy link
Collaborator

GuySartorelli commented Feb 14, 2023

Can you please provide step by step instructions? I'm unable to get the "before" screenshot even with that configuration. I only ever get "after".

Maybe relevant, here's the versions of things I have installed when I can't reproduce the original problem:
dnadesign/silverstripe-elemental 4.10.0
dnadesign/silverstripe-elemental-userforms 3.x-dev f51cd57
silverstripe/admin 1.x-dev 304a4d2
silverstripe/cms 4.x-dev 9269356
silverstripe/framework 4.x-dev 1f7adab
silverstripe/frameworktest 0.4.12
silverstripe/session-manager 1.x-dev 75dd7e1
silverstripe/userforms 5.14.0
symbiote/silverstripe-gridfieldextensions 3.5.0

@kinglozzer
Copy link
Author

My bad, my framework version was locked to 4.11... turns out another genius has already fixed this in framework 😉 silverstripe/silverstripe-framework#10297

@kinglozzer kinglozzer closed this Feb 15, 2023
@kinglozzer kinglozzer deleted the recipients-config branch February 15, 2023 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants